Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cancel Mission on STOP #18

Merged
6 commits merged into from
Jul 17, 2024
Merged

Conversation

lowellausen
Copy link
Member

After some recent re-evaluations we have thought that it might make more sense for the Mission task to actually be cancelled on Stop.

For example, you have a robot doing a mission of visiting many different locations, and then you press a big red Stop button: you expect the robot to stop, not to just skip the current location where it was going to and proceed to the next one. If you want to cancel only the current location, you do have the option to cancel that one task specifically, as long as you have allow_skipping enabled

Changes:

  • Changed the Mission specification to cancel_on_stop=True
  • Removed some comments that didn't seem to make too much sense to me (I can add them again or modify if requested)
  • Will skip a unsuccessful if the whole mission wasn't cancelled
  • Will cancel a mission if cancel was requested
  • Will abort a mission if a subtask was unsuccessful and skipping is not allowed

@lowellausen lowellausen force-pushed the cancel-mission-on-stop branch from 0a6360e to 00e4d8e Compare June 26, 2024 15:43
@ghost ghost requested review from Jannkar and jonipol June 26, 2024 15:44
@ghost ghost assigned lowellausen Jun 26, 2024
Copy link
Contributor

@jonipol jonipol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Jannkar
Copy link
Collaborator

Jannkar commented Jun 30, 2024

Sounds good. I also agree that Missions in general should be cancelled on STOP command.

Copy link
Collaborator

@Jannkar Jannkar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests needed an approval to be ran automatically, so did that now. It seems like two of them are currently failing.

@Jannkar
Copy link
Collaborator

Jannkar commented Jul 5, 2024

The changes look great now! I've emailed you about one more thing to discuss. Once we resolve that one, this one is good to go.

@lowellausen
Copy link
Member Author

The changes look great now! I've emailed you about one more thing to discuss. Once we resolve that one, this one is good to go.

Hey! Not sure if I got your email 🤔

@ghost ghost merged commit 3052660 into Karelics:main Jul 17, 2024
1 check passed
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants